Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run nightly build only if repository has changes #112

Merged
merged 7 commits into from
May 16, 2024

Conversation

adpe
Copy link
Contributor

@adpe adpe commented May 4, 2024

This is a rework from the solution #110, and it's better.

It solves a lot of problems which can occur based on the Git commits, see this comment on Stack Overflow https://stackoverflow.com/a/76596449/7262345

Furthermore, I've removed your hard-coded username, as I want also be able to run the workflow on the fork. I've tried it out and I was able to build and push to DockerHub and also verify, that these changes should work.

Try it out! 🚀 I'm happy to hear your feedback.

Furthermore, it's enough to add a secret for the secrets.DOCKERHUB_TOKEN. If the GitHub and Docker username is the same, we don't need to define it as a secret.

If you want to have it dynamically, why you then hard-code the tag and repository? In case we need that, I would suggest making the whole thing dynamically and add multiple environment variables in a new PR, so that tags, repository, username, repository owner check.

@adpe
Copy link
Contributor Author

adpe commented May 4, 2024

Btw. you can use https://nektosact.com/ to run GitHub actions locally 💪

@adpe
Copy link
Contributor Author

adpe commented May 4, 2024

With this two rounds you would see, how it works in action

With this solution, you will only build your images nightly, if there were commit changes. So it doesn't matter, if the latest commit is 24 hours ago or not, as if you want to build if there are any changes.

@McPringle
Copy link
Owner

As I interpret your changes, the nightly now runs automatically in every fork of the main repo. That this is desired should only very rarely be the case. On the one hand, not everyone who creates a fork of the repo also has an account on Docker Hub, so that an error occurs when pushing. Secondly, it is not in the interests of users to flood Docker Hub with a large number of Apus images with different users.

Your other customization of checking when the last build ran, on the other hand, I think is very good.

@adpe
Copy link
Contributor Author

adpe commented May 8, 2024

Thanks for your answer. We could also only run the build job, if the user configured a DOCKERHUB_TOKEN in the secrets. So this should not be a problem, and would be configurable and not hard-coded in the file.

Probably we could also configure, on which branches this workflow should run or under which conditions.

@adpe
Copy link
Contributor Author

adpe commented May 8, 2024

Furthermore, as GitHub writes here https://docs.github.com/en/actions/using-workflows/disabling-and-enabling-a-workflow the scheduled workflow are disabled on forks. The user must activate it by themselves. So if the user does choose that, I think it's okay that these are triggered then.

@McPringle
Copy link
Owner

I would prefer to add a condition to run only when DOCKERHUB_TOKEN is available, as you suggested. I don't trust GitHub's statement because my fork of the Java Bubble repository runs the scheduled nightly by default and produces errors without me having enabled this feature.

@adpe adpe force-pushed the features/optimize-nightly-build branch 7 times, most recently from 1e7b4d4 to 36308d7 Compare May 13, 2024 17:26
@adpe adpe force-pushed the features/optimize-nightly-build branch from 36308d7 to 7def7b4 Compare May 13, 2024 17:29
@adpe
Copy link
Contributor Author

adpe commented May 13, 2024

I've now refactored the solution and tried it on my personal branch:

Check out the changes and thanks for your feedback!

@McPringle McPringle merged commit fb102a2 into McPringle:main May 16, 2024
2 checks passed
@adpe adpe deleted the features/optimize-nightly-build branch May 17, 2024 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants